Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve: Resolve visibilities on fields with non-builtin attributes #67106

Merged
merged 3 commits into from
Dec 10, 2019

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 6, 2019

Follow-up to #66669.

The first commit is primary (and also a backport candidate), the other ones are further cleanups.
In this case it's not strictly necessary to avoid reporting errors during speculative resolution because 1) all visibilities are resolved non-speculatively sooner or later and 2) error reporting infrastructure merges identical errors with identical spans anyway.

Fixes #67006
r? @matthewjasper

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2019
@Centril Centril added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 6, 2019
@Centril
Copy link
Contributor

Centril commented Dec 6, 2019

The first commit is primary (and also a backport candidate)

Beta nominated for this ^.

Copy link
Contributor

@matthewjasper matthewjasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone wondering what the fix is here: placeholders have inherited visibility for named fields, so the visibility of the actual field isn't being resolved for them, and so no error is reported.

r=me with the comment below addressed

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Dec 9, 2019

@petrochenkov to be clear, the beta-nomination is solely on commit 1a89f4d4d218398c0c8f99c5dabfdb8f4895fedf, right?

@petrochenkov
Copy link
Contributor Author

@pnkfelix
Yes.

@petrochenkov
Copy link
Contributor Author

@bors r=matthewjasper

The exact commit hashes have changed due to rebase and added tests, but the commit to backport is still the first one.

@bors
Copy link
Contributor

bors commented Dec 9, 2019

📌 Commit 5f6267c has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 9, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Dec 9, 2019
resolve: Resolve visibilities on fields with non-builtin attributes

Follow-up to rust-lang#66669.

The first commit is primary (and also a backport candidate), the other ones are further cleanups.
In this case it's not strictly necessary to avoid reporting errors during speculative resolution because 1) all visibilities are resolved non-speculatively sooner or later and 2) error reporting infrastructure merges identical errors with identical spans anyway.

Fixes rust-lang#67006
r? @matthewjasper
bors added a commit that referenced this pull request Dec 10, 2019
Rollup of 11 pull requests

Successful merges:

 - #66892 (Format libcore with rustfmt (including tests and benches))
 - #67106 (resolve: Resolve visibilities on fields with non-builtin attributes)
 - #67113 (Print the visibility in `print_variant`.)
 - #67115 (Simplify `check_decl_no_pat`.)
 - #67119 (libstd miri tests: avoid warnings)
 - #67125 (Added ExactSizeIterator bound to return types)
 - #67138 (Simplify `Layout::extend_packed`)
 - #67145 (fix miri step debug printing)
 - #67149 (Do not ICE #67123)
 - #67155 (Move `Layout`s instead of binding by reference)
 - #67169 (inline some common methods on OsStr)

Failed merges:

r? @ghost
@bors bors merged commit 5f6267c into rust-lang:master Dec 10, 2019
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 12, 2019
resolve: Always resolve visibilities on impl items

Fixes rust-lang#64705.

Similarly to rust-lang#67106 this was an issue with visitor discipline.
Impl items were visited as a part of visiting `ast::ItemKind::Impl`, but they should be visit-able in isolation  from their parents as well, because that's how they are visited when they are expanded from macros.

I've checked that all the remaining `resolve_visibility` calls are used correctly.

r? @matthewjasper
@pnkfelix
Copy link
Member

discussed at T-compiler meeting. accepted for beta backport.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 12, 2019
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 14, 2019
bors added a commit that referenced this pull request Dec 14, 2019
[beta] Beta backports

Backporting the following pull requests:

 * resolve: Always resolve visibilities on impl items #67236
 * resolve: Resolve visibilities on fields with non-builtin attributes #67106
 * E0023: handle expected != tuple pattern type #67044
 * Fix `unused_parens` triggers on macro by example code #66983
 * Fix some issues with attributes on unnamed fields #66669
 * Ensure that we get a hard error on generic ZST constants if their bodies #67134 (via #67297)

Some of these conflicted on merge, I resolved where possible, sometimes by cherry-picking a commit or two more from the relevant PRs. Since those changes are necessary though for backport to proceed (otherwise not even std/core compile), seems fine -- they're fairly minor cleanups anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants